fix(tee): fix race condition in batch locking #3342
Conversation
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
a86fc98
to
e95cb27
Compare
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
e95cb27
to
46dcfde
Compare
After [scaling][1] [zksync-tee-prover][2] to two instances/replicas on Azure for azure-stage2, azure-testnet2, and azure-mainnet2, we started experiencing [duplicated proving for some batches][3]. While this is not an erroneous situation, it is wasteful from a resource perspective. This was due to a race condition in batch locking. This PR fixes the issue by adding atomic batch locking. [1]: https://github.com/matter-labs/gitops-kubernetes/pull/7033/files [2]: https://github.com/matter-labs/zksync-era/blob/aaca32b6ab411d5cdc1234c20af8b5c1092195d7/core/bin/zksync_tee_prover/src/main.rs [3]: https://grafana.matterlabs.dev/goto/M1I_Bq7HR?orgId=1
pbeza
force-pushed
the
tee/fix/atomic-batch-locking
branch
from
46dcfde
to
7d96c1c
Compare
There was a problem hiding this comment.
Dumb question: How is the locking made atomic in this PR? AFAIU, the first SELECT statement, if queried concurrently, can still return the same L1 batch number unless some kind of row-level locking is implemented (cf. SELECT FOR UPDATE SKIP LOCKED in this contract verifier query). I'm not even sure the UPDATE query will fail for the transaction committed last in case of a race (maybe it would with serialization isolation level, but I'd argue that erroring is not the best cause of action here; row-level locks seem to work better).
Not a dumb question at all! The dumb one here was me! ;P I totally misunderstood what SQL transactions can actually handle in this context. Had to brush up on the finer details of SQL locking. Thanks for steering me in the right direction! These two links were super helpful: |
|
@slowli, I’ve addressed your code review comments. Take a look when you get a chance. It’s kinda hard to test properly without deploying it to |
|
@slowli, @haraldh suggested locking the entire Let me know if this more fine-grained locking approach still works for you, or if we’re missing something – or maybe there’s an easier way we haven’t considered. |
This reverts commit a1f99ab.
Commit a7dc0ed (PR #3342) was supposed to fix a race condition in batch locking by introducing SQL row-locking, but it didn't work as expected. Now we are switching back to coarser-grained table-level locking as [originally suggested][1] by Harald. The original fix was hard to test unless deployed to `stage` due to the undeterministic nature of the problem, so we needed to merge it to the `main` branch to properly test it. [1]: #3342 (comment)
What ❔
After scaling
zksync-tee-proverto two instances/replicas on Azure forazure-stage2,azure-testnet2, andazure-mainnet2, we started experiencing duplicated proving for some batches.While this is not an erroneous situation, it is wasteful from a resource perspective. This was due to a race condition in batch locking. This PR fixes the issue by adding atomic batch locking.
Why ❔
To fix the bug that only activates after running
zksync-tee-proveron multiple instances.Checklist
zkstack dev fmtandzkstack dev lint.